pkg/wshrpc/wshremote/sysinfo_test.go (1)

20-28: Consider adding verification for tool availability tests.

While logging the availability is useful, consider adding assertions to verify expected behavior in CI environments or when specific tools are known to be present.

pkg/wshrpc/wshremote/sysinfo.go (6)

189-189: Consider using debug-level logging for GPU detection.

This log statement will execute every second for each GPU, potentially flooding the logs. Consider using debug-level logging or removing it.

-		log.Printf("Found macOS GPU: %s", name)
+		// Only log in debug mode or remove entirely

---

399-399: Document the GPU memory estimation heuristic.

The 10% estimation of system memory for GPU is arbitrary and could be misleading. Consider adding a comment explaining this is a rough fallback estimate.

 			// Estimate GPU memory as a fraction of system memory
-			// This is a rough estimate and varies by GPU
+			// This is a rough fallback estimate (10% of system RAM) when actual VRAM info is unavailable
+			// Actual GPU memory varies significantly and this should be treated as unreliable
 			return totalGB * 0.1 // Assume 10% of system memory for GPU

---

463-463: Consider using debug-level logging for Windows GPU detection.

Similar to the macOS implementation, this will log every second for each GPU. Consider using debug-level logging or removing it.

-		log.Printf("Found Windows GPU: %s (%.2f GB total, %.2f GB used)", gpu.Name, gpu.MemTotal, memUsed)
+		// Only log in debug mode or remove entirely

---

415-427: Consider extracting PowerShell scripts to separate files or constants.

Large embedded PowerShell scripts are difficult to maintain and test. Consider extracting them to constants or external files for better maintainability.

+const psGetGpuInfoScript = `
+	$gpus = Get-WmiObject -Class Win32_VideoController | Where-Object { $_.Name -notlike "*Basic*" -and $_.Name -notlike "*Standard*" }
+	$gpuInfo = @()
+	foreach ($gpu in $gpus) {
+		$gpuInfo += [PSCustomObject]@{
+			Name = $gpu.Name
+			AdapterRAM = $gpu.AdapterRAM
+			VideoProcessor = $gpu.VideoProcessor
+			DriverVersion = $gpu.DriverVersion
+		}
+	}
+	$gpuInfo | ConvertTo-Json -Compress
+`
 
 func getWindowsGpuData() ([]GpuData, error) {
 	// ...
-	psCommand := `
-		$gpus = Get-WmiObject -Class Win32_VideoController | Where-Object { $_.Name -notlike "*Basic*" -and $_.Name -notlike "*Standard*" }
-		// ... rest of script
-	`
+	psCommand := psGetGpuInfoScript

---

641-643: Consider logging GPU detection failures for debugging.

Silently ignoring GPU detection errors makes troubleshooting difficult. Consider logging the error at debug level.

 if err != nil || len(gpus) == 0 {
+	if err != nil {
+		log.Printf("Failed to get GPU data: %v", err)
+	}
 	return
 }

---

697-697: Remove redundant inline comment.

The function name getGpuData is self-documenting. The inline comment adds no value.

-	getGpuData(values) // Add this line to get GPU data
+	getGpuData(values)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1725
File: pkg/remote/fileshare/wavefs/wavefs.go:441-494
Timestamp: 2025-01-29T04:21:11.649Z
Learning: The `CopyRemote` function in WaveTerm's file operations has proper error handling that prevents partial writes by failing fast and using context cancellation. Each step (path cleaning, file operations, tar reading/writing) is guarded by error checks that prevent proceeding with writes on error.

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/awsconn/awsconn.go:32-33
Timestamp: 2025-01-22T01:02:32.621Z
Learning: The AWS connection code path in pkg/remote/awsconn/awsconn.go is currently inactive, so temporary file cleanup is not a critical concern at this time.

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.

pkg/wshrpc/wshremote/sysinfo.go (2)

50-61: Platform detection correctly implemented!

The function now properly uses runtime.GOOS for cross-platform detection, which resolves the previous issue with uname not being available on Windows.

---

108-110: Inconsistent error handling for parsing GPU metrics.

While errors are handled for index and util parsing (lines 98-107), the parsing of memUsed, memTotal, and temp still ignores errors. This inconsistency could lead to confusion about which metrics are reliable.

Apply consistent error handling:

-memUsed, _ := strconv.ParseFloat(strings.TrimSpace(fields[2]), 64)
-memTotal, _ := strconv.ParseFloat(strings.TrimSpace(fields[3]), 64)
-temp, _ := strconv.ParseFloat(strings.TrimSpace(fields[4]), 64)
+memUsed, err := strconv.ParseFloat(strings.TrimSpace(fields[2]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi memory used: %v", err)
+    memUsed = 0
+}
+memTotal, err := strconv.ParseFloat(strings.TrimSpace(fields[3]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi memory total: %v", err)
+    memTotal = 0
+}
+temp, err := strconv.ParseFloat(strings.TrimSpace(fields[4]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi temperature: %v", err)
+    temp = 0
+}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1807
File: pkg/wshrpc/wshremote/wshremote.go:718-726
Timestamp: 2025-01-23T06:09:05.334Z
Learning: In Go, using a single if statement with the `&&` operator (e.g. `if x != nil && x.Field != nil`) is the idiomatic way to handle multiple nil checks. Avoid suggesting nested if statements as they make the code more verbose without adding value.

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.

pkg/wshrpc/wshremote/sysinfo.go (1)

49-61: Platform detection correctly implemented.

The function now properly uses runtime.GOOS for reliable cross-platform detection, addressing the previous Windows compatibility issue.

pkg/wshrpc/wshremote/sysinfo.go (4)

108-110: Complete error handling for remaining fields.

While error handling was added for index and utilization parsing, the memory and temperature parsing still ignores errors. For consistency and robustness, consider adding error handling for these fields as well.

-memUsed, _ := strconv.ParseFloat(strings.TrimSpace(fields[2]), 64)
-memTotal, _ := strconv.ParseFloat(strings.TrimSpace(fields[3]), 64)
-temp, _ := strconv.ParseFloat(strings.TrimSpace(fields[4]), 64)
+memUsed, err := strconv.ParseFloat(strings.TrimSpace(fields[2]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi memory used: %v", err)
+    memUsed = 0
+}
+memTotal, err := strconv.ParseFloat(strings.TrimSpace(fields[3]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi memory total: %v", err)
+    memTotal = 0
+}
+temp, err := strconv.ParseFloat(strings.TrimSpace(fields[4]), 64)
+if err != nil {
+    log.Printf("Error parsing nvidia-smi temperature: %v", err)
+    temp = 0
+}

---

125-179: Good improvements with error handling, but consider output format robustness.

The error handling has been properly implemented as requested in previous reviews. However, the regex pattern for parsing rocm-smi output might be fragile if the output format changes between versions.

Consider adding more flexible parsing or fallback mechanisms for different rocm-smi output formats to improve robustness across AMD driver versions.

---

254-280: Improve macOS GPU utilization detection reliability.

The current approach to parse iostat output for GPU utilization is quite fragile. The parsing logic looks for lines containing "gpu" or "GPU" but iostat typically doesn't provide GPU-specific information in most versions.

Consider returning 0 or using a more reliable method for GPU utilization on macOS, as this parsing is likely to fail in most cases:

 func getMacGpuUtilization() float64 {
-	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
-	defer cancel()
-
-	// Try to get GPU utilization using iostat
-	cmd := exec.CommandContext(ctx, "iostat", "-n", "1", "1")
-	output, err := cmd.Output()
-	if err != nil {
-		return 0
-	}
-
-	// Parse iostat output for GPU utilization
-	lines := strings.Split(string(output), "\n")
-	for _, line := range lines {
-		if strings.Contains(line, "gpu") || strings.Contains(line, "GPU") {
-			fields := strings.Fields(line)
-			if len(fields) >= 2 {
-				if util, err := strconv.ParseFloat(fields[1], 64); err == nil {
-					return util
-				}
-			}
-		}
-	}
-
-	return 0
+	// iostat doesn't typically provide GPU utilization on macOS
+	// Return 0 to indicate unavailable data
+	return 0
 }

---

468-510: Improve Windows GPU output parsing robustness.

The parseWindowsGpuOutput function uses regex to parse JSON-like output, but this approach is fragile. If the PowerShell command actually returns valid JSON, consider using proper JSON parsing.

+import "encoding/json"

 func parseWindowsGpuOutput(output string) []WindowsGpuInfo {
 	var gpuList []WindowsGpuInfo
 
-	// Try to parse as JSON array
 	if strings.TrimSpace(output) == "" {
 		return gpuList
 	}
 
-	// Simple JSON parsing for the PowerShell output
-	// The output should be a JSON array of GPU objects
-	lines := strings.Split(output, "\n")
-	for _, line := range lines {
-		line = strings.TrimSpace(line)
-		if line == "" {
-			continue
-		}
-
-		// Extract GPU name and memory info using regex
-		nameRe := regexp.MustCompile(`"Name":\s*"([^"]+)"`)
-		adapterRamRe := regexp.MustCompile(`"AdapterRAM":\s*(\d+)`)
-
-		nameMatches := nameRe.FindStringSubmatch(line)
-		ramMatches := adapterRamRe.FindStringSubmatch(line)
-
-		if len(nameMatches) >= 2 && len(ramMatches) >= 2 {
-			name := nameMatches[1]
-			if ramSize, err := strconv.ParseInt(ramMatches[1], 10, 64); err == nil {
-				// Convert bytes to GB
-				memTotal := float64(ramSize) / (1024 * 1024 * 1024)
-
-				gpuInfo := WindowsGpuInfo{
-					Name:     name,
-					MemTotal: memTotal,
-					MemUsed:  0,
-				}
-				gpuList = append(gpuList, gpuInfo)
-			}
-		}
-	}
+	// Try proper JSON parsing first
+	var rawGpus []map[string]interface{}
+	if err := json.Unmarshal([]byte(output), &rawGpus); err == nil {
+		for _, gpu := range rawGpus {
+			if name, ok := gpu["Name"].(string); ok {
+				if ramInterface, ok := gpu["AdapterRAM"]; ok {
+					var ramSize int64
+					switch v := ramInterface.(type) {
+					case float64:
+						ramSize = int64(v)
+					case string:
+						if parsed, err := strconv.ParseInt(v, 10, 64); err == nil {
+							ramSize = parsed
+						}
+					}
+					
+					if ramSize > 0 {
+						memTotal := float64(ramSize) / (1024 * 1024 * 1024)
+						gpuInfo := WindowsGpuInfo{
+							Name:     name,
+							MemTotal: memTotal,
+							MemUsed:  0,
+						}
+						gpuList = append(gpuList, gpuInfo)
+					}
+				}
+			}
+		}
+		return gpuList
+	}
+	
+	// Fallback to regex parsing if JSON parsing fails
+	// ... existing regex logic ...
 
 	return gpuList
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.

Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.

frontend/app/view/sysinfo/sysinfo.tsx (2)

68-94: Add GPU temperatures to Plot Types and harden GPU index parsing (optional).

Current GPU plot types look good. Consider adding a dedicated “GPU Temp” plot and make index parsing more robust to avoid NaN comparisons if keys ever vary.

     "All GPU": function (dataItem: DataItem): Array<string> {
-        return Object.keys(dataItem)
-            .filter((item) => item.startsWith("gpu:") && item.includes(":util"))
-            .sort((a, b) => {
-                const valA = parseInt(a.replace("gpu:", "").replace(":util", ""));
-                const valB = parseInt(b.replace("gpu:", "").replace(":util", ""));
-                return valA - valB;
-            });
+        const idx = (k: string) => Number(k.match(/^gpu:(\d+):/)?.[1] ?? Number.POSITIVE_INFINITY);
+        return Object.keys(dataItem)
+            .filter((item) => item.startsWith("gpu:") && item.includes(":util"))
+            .sort((a, b) => idx(a) - idx(b));
     },
+    "GPU Temp": function (dataItem: DataItem): Array<string> {
+        const idx = (k: string) => Number(k.match(/^gpu:(\d+):/)?.[1] ?? Number.POSITIVE_INFINITY);
+        return Object.keys(dataItem)
+            .filter((item) => item.startsWith("gpu:") && item.includes(":temp"))
+            .sort((a, b) => idx(a) - idx(b));
+    },

---

103-104: Use GPU color for GPU aggregate metric for visual consistency.

The “gpu” meta reuses the CPU color. Prefer the GPU color token.

-    gpu: defaultCpuMeta("GPU %"),
+    gpu: { ...defaultCpuMeta("GPU %"), color: "var(--sysinfo-gpu-color)" },

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.